feat(tree): Allow event deferral during transactions#27238
feat(tree): Allow event deferral during transactions#27238Josmithr wants to merge 13 commits intomicrosoft:mainfrom
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (479 lines, 12 files), I've queued these reviewers:
How this works
|
There was a problem hiding this comment.
Pull request overview
Adds opt-in buffering of SimpleTree change events during synchronous runTransaction calls so that listeners observe a single coalesced notification after commit, and no notifications for synchronous rollbacks.
Changes:
- Add
deferEvents?: booleantoRunTransactionParamsand wrapTreeCheckout.runTransactionwithwithBufferedTreeEventswhen enabled. - Extend
withBufferedTreeEvents/kernel event buffering to support discarding buffered events (used on rollback). - Add new SharedTreeView tests validating buffered/coalesced
nodeChanged/treeChangedbehavior, including nested transaction scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts | Adds coverage for deferred/coalesced nodeChanged/treeChanged behavior and rollback semantics. |
| packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts | Adds discard support to the kernel’s event buffering so buffered events can be dropped instead of flushed. |
| packages/dds/tree/src/simple-tree/api/transactionTypes.ts | Extends the public transaction params API with the new deferEvents option and its documentation. |
| packages/dds/tree/src/shared-tree/treeCheckout.ts | Implements deferEvents support by buffering events around the transaction and discarding on synchronous rollback. |
Josmithr
changed the title
…github.com/Josmithr/FluidFramework into tree/allow-event-buffering-in-transactions
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
| * @param options - Optional configuration. See {@link WithBufferedTreeEventsOptions}. | ||
| */ | ||
| export function withBufferedTreeEvents(callback: () => void): void { | ||
| export function withBufferedTreeEvents<TResult>( |
There was a problem hiding this comment.
I think it might be cleaner to have the primary callback passed to withBufferedTreeEvents return true/false to determine discard behavior, rather than having this additional parameter. It would make sense since the whole point of this method is to buffer events, and the return value of the main callback would be controlling whether that happens or not.
The downside is that then you couldn't return the return value of the transaction directly, so you might have to add a few extra lines at the call site to capture the return value within the callback and have it outside. But the call site is already split up anyway to accommodate the current scheme:
const result = callback();
discard = options?.shouldDiscard?.(result) === true;
return result;
It's not like it's a super clean one liner, so I don't think it'd make it too much messier. And then this whole interface would get much simpler and self-describing. What do you think?
| @@ -542,6 +582,22 @@ class KernelEventBuffer implements Listenable<KernelEvents> { | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Can this method delegate to discard() now?
It clears childrenChangeBuffer, fieldMarksBuffer, and invalidatedFieldMarkKeys - sometimes.
It also clears subTreeChangedBuffer - sometimes.
But it appears to me that the "sometimes" is purely an optimization - i.e., the only time it doesn't clear them is because they are already clear. If that is true, then I think the cleanliness of removing all those clears and simply calling discard() at the end of flush() is worth it. I imagine that clear() on an empty map/set in JS has negligible perf cost.
| } | ||
|
|
||
| // @alpha @input | ||
| export interface RunTransactionAsyncParams extends RunTransactionParams { |
There was a problem hiding this comment.
IMO, no reason to add this until we need it. runTransactionAsync can just take the more general RunTransactionParams for now. We can always add a type like this later if necessary.
There was a problem hiding this comment.
That was how I originally had it, but it seemed a bit weird to me for runTransaction to take RunTransactionSyncParams, while runTransactionAsync takes just RunTransactionParams.
But maybe it would make more sense to just do some type renames (these are alpha APIs after all).
Adds a
deferEventsflag torunTransaction, which allows users to opt-into deffering events until after the transaction has completed.